v1.18: Treat super low staked as unstaked in streamer QOS (backport of #701)#733
v1.18: Treat super low staked as unstaked in streamer QOS (backport of #701)#733lijunwangs merged 1 commit intov1.18from
Conversation
* Treat super low staked with QOS of unstaked * simplify * address some comment from Pankaj (cherry picked from commit 92ebf0f)
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## v1.18 #733 +/- ##
=======================================
Coverage 81.5% 81.5%
=======================================
Files 827 827
Lines 224833 224835 +2
=======================================
+ Hits 183440 183448 +8
+ Misses 41393 41387 -6 |
|
Just making sure this was intentional (I probably missed the conversation):
|
Yes. To be eligible to send 1 stream per throttle interval. The PPS difference in master and v1.17 does not make sense to me. In the end we probably we will stick with one value. |
Oh, should we land on a desired value now and make sure we have consistent values across the branches ? |
Since this PR is not changing the PPS value, we should let this merge. The question makes sense and we should identify a number that's same across branches. |
Yes, but that should be addressed a separate PR -- #705. I closed it for now. Pending further research into it. Let's table this for now. But the core design goal remains the same. Minimal threshold is at least 1 stream per throttle window assuming proportional bandwidth allocation. |
| let min_stake_ratio = | ||
| 1_f64 / (MAX_STREAMS_PER_MS * STREAM_THROTTLING_INTERVAL_MS) as f64; |
There was a problem hiding this comment.
Why isn't this defined as "const literal" and then maybe const_assert or debug_assert? like:
https://github.com/anza-xyz/agave/blob/baed522d2/local-cluster/src/integration_tests.rs#L208
right now someone can change any of the two referenced constants unbeknown of how much it impacts this threshold here.
There was a problem hiding this comment.
The intention is really trying to get the max streams we allow in the throttle interval. So if the value is changed, we really want the change as well. But I think it makes sense to keep these constants together so people might realize the impact. I will address in master.
…anza-xyz#701) (anza-xyz#733) Treat super low staked as unstaked in streamer QOS (anza-xyz#701) * Treat super low staked with QOS of unstaked * simplify * address some comment from Pankaj (cherry picked from commit 92ebf0f) Co-authored-by: Lijun Wang <83639177+lijunwangs@users.noreply.github.com>
6 participants
Problem
staked nodes with low stakes can abuse the system to get disproportional bandwidth.
Summary of Changes
Treat it as unstaked node for the with stake ratio stake/total_stake < 1 / (max packet per 100 ms)
Fixes #
This is an automatic backport of pull request #701 done by [Mergify](https://mergify.com).